Add new security analyzers, remediation module#273
Conversation
- Added behavioral fingerprinting analyzer node - Added cross-skill dependency analyzer node - Added prompt injection resilience analyzer node - Integrated new nodes into `__init__.py` (ANALYZER_NODE_IDS and ANALYZER_NODES) - Created `remediation.py` and `watcher.py` to handle automated responses and live monitoring - Updated `cli.py` and `pattern_defaults.py` to support the new features
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
[Automated SkillSpector Review]
Summary: Adds three new analyzer modules (behavioral_fingerprint, cross_skill_dependency, prompt_injection_resilience), a remediation.py auto-fix module, and a watcher.py polling watch mode — 1,056 added lines across 5 new files, 0 modifications to existing files, 0 tests.
Blockers
- PR description does not match the diff / all new code is dead code. The description claims the new nodes were "Integrated ... into
__init__.py(ANALYZER_NODE_IDS and ANALYZER_NODES)" and thatcli.pyandpattern_defaults.pywere updated. None of those files are touched in this PR. The analyzers are never registered (verified againstsrc/skillspector/nodes/analyzers/__init__.pyat main),remediation.py's promisedskillspector fixcommand andwatcher.py'sskillspector watchcommand have no CLI wiring, and nothing imports any of the five new modules. As submitted, no behavior changes at all. - No tests. Repo convention is per-analyzer tests under
tests/nodes/analyzers/(e.g.test_behavioral_ast.py,test_static_patterns.py). 1,056 lines of new detection/remediation logic ship with zero coverage. Notetests/nodes/analyzers/test_registry.pypins the exact analyzer registry list — actual integration will require updating it. - CI lint failure (ruff
F401): unused imports —get_context_from_lines,get_source_segmentinbehavioral_fingerprint.py;textwrapandPathinremediation.py. prompt_injection_resiliencefires on every file of every type.node()maps anything that isn't.md/.txtto"other", andanalyze()accepts"other", so the docstring's "only analyze markdown and text files" is never enforced — Python/JSON/YAML files are graded for "instruction boundaries". Worse, IR1/IR3/IR4 are absence-based and per-file: every benign file over 200 chars that lacks security phrasing yields MEDIUM/LOW findings, flooding every scan and inflating risk scores. IR2's pattern also inverts meaning:(?:always|never)\s+(?:validate|verify|sanitize|check)...flags "Always validate the input" — a protective instruction — as "trusts user input without validation" at 0.8 confidence (verified).cross_skill_dependencyfalse-positive machinery is broken. (a)_skill_name_from_path("skill-a/SKILL.md")returns"SKILL"(the file stem), not"skill-a", so the CS1 self-reference exclusion never works. (b) The multi-skill gate collects every intermediate directory as a "skill name" — a single skill with ascripts/subdir yields{"my-skill", "scripts"}and passes the>= 2gate; the generic pattern(?:skill|agent|tool)[/\s]+(name)then matches prose like "tool scripts" (verified). CS3 additionally flags any mention of "mutex"/"lockfile"/"barrier" anywhere.behavioral_fingerprintURL detection matches plain English. The(?:POST|GET|PUT|DELETE|PATCH)\s+\S+alternative is compiled withre.IGNORECASE, so "get started,", "put the", "delete old" in any markdown are reported as external network endpoints under FP3 (verified).
Non-blocking issues
behavioral_fingerprint.node()parses every Python file twice, and the computed fingerprint is only logged — the module's stated purpose (known-bad comparison, drift detection, threat-intel sharing) is unimplemented.- FP4 flags
os+jsonimports as a "serialization + execution" dangerous combination — true for nearly every Python file;osis also double-counted in bothexecutionandenvgroups. - FP2's env-var regex requires a paren after
[, soos.environ["API_KEY"]subscript access is missed (verified); only.get()/.pop()forms are detected. - New rule IDs (FP1–FP4, CS1–CS3, IR1–IR5) have no entries in
pattern_defaults.py, despite the PR claiming that file was updated. remediation.pydesign: regex fixes apply file-wide rather than anchored to the finding location; the P2 fix strips all HTML comments including legitimate ones; and "sanitizing" malicious content with narrow regexes risks a false sense of safety — a flagged skill can remain malicious after "remediation". For a security tool, quarantine/reject semantics deserve discussion before an auto-fix ships.watcher.py: docstring says debounce waits after a change, but the implementation triggersdebounceseconds after the first change even while edits continue; the whole tree is re-hashed every 2s poll; duplicated alternative(?:include|include|contain)typo exists inprompt_injection_resilience.py's output-guard pattern.
Requested changes
Either split this into reviewable, integrated units or complete the integration described in the PR body: register the nodes, wire the CLI commands, add pattern_defaults.py entries, update test_registry.py, add tests for each new rule, fix the verified FP bugs above, and remove unused imports.
| return findings | ||
|
|
||
|
|
||
| def _skill_name_from_path(file_path: str) -> str: |
There was a problem hiding this comment.
_skill_name_from_path("skill-a/SKILL.md") returns "SKILL" (the last path part is the file name, and its stem is returned), not the skill directory "skill-a". The CS1 self-reference exclusion that compares against this value therefore never matches, so a skill referencing its own name is flagged as a cross-skill reference. Iterate over directory parts only (exclude the final path component).
| for path in components: | ||
| parts = path.replace("\\", "/").split("/") | ||
| for part in parts[:-1]: | ||
| if part and not part.startswith("."): |
There was a problem hiding this comment.
This collects every intermediate directory component as a "skill name": a single skill laid out as my-skill/scripts/run.py produces {"my-skill", "scripts"}, passing the len(skill_names) < 2 gate below. Combined with the generic (?:skill|agent|tool)[/\s]+(name) reference pattern, prose like "tool scripts" then triggers CS1 MEDIUM findings on single-skill scans (verified). Skill roots should come from the multi-skill discovery logic (see multi_skill.py), not from arbitrary path segments.
| _URL_PATTERN = re.compile( | ||
| r"https?://[^\s\"']+|" | ||
| r"wss?://[^\s\"']+|" | ||
| r"(?:POST|GET|PUT|DELETE|PATCH)\s+[^\s\"']+", |
There was a problem hiding this comment.
The (?:POST|GET|PUT|DELETE|PATCH)\s+[^\s\"']+ alternative is compiled with re.IGNORECASE, so ordinary prose matches: "get started,", "put the", "delete old" are all captured as URLs and reported as external network endpoints under FP3 (verified). Drop IGNORECASE for the HTTP-verb alternative (verbs are conventionally uppercase) and/or require a path-like operand (e.g. /\S+ or a scheme).
| tag = [_TAG] | ||
|
|
||
| # Only analyze markdown and text files (skill instruction files) | ||
| if file_type not in ("markdown", "text", "other"): |
There was a problem hiding this comment.
node() maps every non-.md/.txt suffix to "other", and "other" is accepted here, so this early-return never filters anything — Python, JSON, and YAML files are all graded for instruction boundaries and output guards, contradicting the docstring. Since IR1/IR3/IR4 fire on the absence of phrasing, every benign file >200 chars produces MEDIUM/LOW findings, flooding scan results. Restrict to skill instruction files (SKILL.md) and consider evaluating absence once per skill, not per file.
| # Patterns indicating trust in user content | ||
| _TRUSTING_PATTERNS = [ | ||
| (re.compile(r"(?:trust|believe|assume)\s+(?:the\s+)?(?:user|input|message)", re.IGNORECASE), 0.7), | ||
| (re.compile(r"(?:always|never)\s+(?:validate|verify|sanitize|check)\s+(?:the\s+)?(?:input|user)", re.IGNORECASE), 0.8), |
There was a problem hiding this comment.
This pattern flags "Always validate the input" — a protective instruction — as IR2 "Skill trusts user input without validation" at confidence 0.8 (verified). The always alternative inverts the rule's intent; only negated forms ("never validate", "don't verify") indicate trusting behavior.
|
|
||
| import difflib | ||
| import re | ||
| import textwrap |
There was a problem hiding this comment.
textwrap (here) and Path (next line) are unused, as are get_context_from_lines/get_source_segment in behavioral_fingerprint.py — these fail ruff F401, which is enabled in pyproject.toml (select includes "F").
2 participants
__init__.py(ANALYZER_NODE_IDS and ANALYZER_NODES)remediation.pyandwatcher.pyto handle automated responses and live monitoringcli.pyandpattern_defaults.pyto support the new features